Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: serialize 'application/x-www-form-urlencoded' with array indices in browser #1589

Closed
wants to merge 1 commit into from
Closed

fix: serialize 'application/x-www-form-urlencoded' with array indices in browser #1589

wants to merge 1 commit into from

Conversation

dsanders11
Copy link
Contributor

This is a quick and dirty PR more for discussion than immediate merging.

Currently the behavior for serializing application/x-www-form-urlencoded payloads seems to differ between Node and the browser, with Node using indices for array values and the browser not. This PR tries to make the behavior consistent, with the assumption that the behavior in Node is the intended behavior.

So, open question on which environment has the intended behavior currently. I see that in Node indices are turned off for serializing the query string, but not for the payload body when it's application/x-www-form-urlencoded.

@niftylettuce
Copy link
Collaborator

niftylettuce commented Aug 26, 2020 •
edited
Loading

I'm wondering if using a package like https://www.npmjs.com/package/qs would be best, as to ensure consistency regardless of Node/Browser changes, e.g. qs.stringify({ a: ['b', 'c', 'd'] }, { indices: true });

@dsanders11
Copy link
Contributor Author

@niftylettuce, that package is already used in the Node code, isn't it? I wasn't sure if it plays nice in the browser environment. If so, that seems like a simpler way to go as it would reduce some code and keep them consistent, as you said.

@niftylettuce
Copy link
Collaborator

Yeah qs works fine in browsers, superagent is actually built to make dist builds for browsers already so should be fine to use it for both. Would love a PR! Would be super super helpful, I'm so stressed for time

@dsanders11 dsanders11 mentioned this pull request Aug 30, 2020
Merged
@dsanders11
Copy link
Contributor Author

@niftylettuce, made PR #1591, which just uses qs and should be quick and easy. Thanks for the feedback.

@dsanders11 dsanders11 closed this Aug 30, 2020
@niftylettuce
Copy link
Collaborator

v6.1.0 has been released to npm, thank you @dsanders11

https://github.com/visionmedia/superagent/releases/tag/v6.1.0

@dsanders11 dsanders11 deleted the serialize-with-indices branch September 27, 2020 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dsanders11 @niftylettuce